Expand useTracking API to accept contextual data #168
Conversation
There was a problem hiding this comment.
This is amazing, thank you!
Also for your question about the API:
const [Track, tracking] = useTracking(data);I actually quite like that idea, I think it's kind of brilliant! It's helpful because it makes "opting-out" of adding the <Track /> component fairly explicit. But two concerns:
- Do you think this API would be awkward? It works for
useState()because in 99% of cases you're definitely going to be pulling off at least the current state (first item in the tuple array), but for tracking the case where you don't need to add contextual tracking data can be fairly common (e.g. at least at all leaf nodes). What do you think? - This would be a breaking change to the existing API. Right now we return an object, this would cause us to return an array. Maybe not a big deal? But it does have me wondering if we should introduce this Hooks API in a backwards compatible way and then in a future semver major release start returning an array instead, especially if we hear of folks forgetting to include
<Track />and being confused why they're missing contextual tracking info? What do you think?
|
@tizmagik Yes, I think you're right about that tuple API. It could be a bit awkward, and I let my head drift off and forget that such a change wouldn't be backward compatible. Also, I wouldn't want to make things more complicated in anticipation of a potential issue that we haven't actually seen users have in the wild. If we're hearing that folks are running into that issue then we can reevaluate. In the meantime, I'll make that update to prevent any changes in the HoC API and do some manual testing to make sure everything looks good! |
There was a problem hiding this comment.
This is brilliant, thanks for doing this!
Two minor things I commented on inline, but I leave it up to you -- I'm fine merging as-is.
One other thing we'll need to do is add some new docs around this. Happy to leave that for a follow-on PR or just create an issue but we'll def need a new section relatively high up on the current README explaining the new Hooks-only capability.
Co-authored-by: Jeremy Gayed <tizmagik@users.noreply.github.com>
|
@tizmagik No prob, it's been fun! Going to make the suggested changes to the tests, and then I think I can take a stab at updating the docs in a separate PR if that seems alright. |
|
FYI, I found a bug while doing some manual testing. Many of the functions in the hook have a dependency on |
|
Ah, good catch. I think this test attempts to get at that (unnecessary renders), but I think wouldn't catch the case you're talking about. |
src/__tests__/hooks.test.js
Outdated
|
|
||
| const wrapper = mount(<App />); | ||
|
|
||
| wrapper.find('button').simulate('click'); |
There was a problem hiding this comment.
Maybe click it a couple times and wrap in act() for good measure? Would be great to also assert that the state is actually updated to confirm.
There was a problem hiding this comment.
Great call on this. Adding a couple extra clicks exposed a bug. I decided not to wrap the clicks in act() because it wasn't making a difference in the result, and it turns out that's because as of React 16.8, if we use mount, enzyme wraps calls to .simulate in act for us. I think we just needed it in the other spot to handle the asynchronicity.
I still want to do some manual testing again before we merge this just to be sure everything is working properly. Should we aim for a Monday merge?
Co-authored-by: Jeremy Gayed <tizmagik@users.noreply.github.com>
This PR could probably use more refinement, but I want to get some early feedback on it. It addresses issue #138. I spent some time considering this API vs having the
Trackcomponent take in props. I think this is the better option because it allows contextual data to be passed into the hook and get back atrackEventfunction that can make use of that data, which would not be possible if the data had to be passed into theTrackcomponent instead.One minor (I think) concern I still have is that it might be too easy for a developer to forget to wrap their markup in the
Trackcomponent, and mistakenly believe that child components are receiving contextual data when they're not. Open question: Would it make any sense to emulate theuseStateAPI, and do something like:That would at least force the developer to very clearly opt out of using
Trackby needing to do:Maybe that's too heavy handed though. I don't know—something worth thinking about perhaps.